-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(imagebuilder): add support for EC2 Image Builder L2 Constructs - Infrastructure Configuration #35882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c34ddf8 to
9f0796e
Compare
4c463b8 to
e6969a9
Compare
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
007c467 to
ab5b1f9
Compare
da30685 to
5c5bf40
Compare
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/infrastructure-configuration.ts
Outdated
Show resolved
Hide resolved
| securityGroupIds: props.securityGroups?.length | ||
| ? props.securityGroups?.map((securityGroup) => securityGroup.securityGroupId) | ||
| : undefined, | ||
| subnetId: props.vpc?.selectSubnets(props.subnetSelection).subnetIds[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If selectSubnets() returns 0 results, this becomes undefined and thus contradicts the user’s intent (where they provided vpc/subnetSelection). We should consider adding validation for this along with a default subnet selection type.
const selectedSubnets = props.vpc ? props.vpc.selectSubnets(props.subnetSelection ?? { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }) : undefined;
if (props.vpc && selectedSubnets && selectedSubnets.subnetIds.length === 0) {
throw new cdk.ValidationError(
'No subnets matched the given subnetSelection for the provided VPC.', this,
);
}
Or we should avoid accepting a VPC without a subnetSelection:
if (props.vpc && !props.subnetSelection) {
throw new cdk.ValidationError(
'A subnetSelection is required when providing a VPC. ' this,
);
}
const selectedSubnets = props.vpc ? props.vpc.selectSubnets(props.subnetSelection!) : undefined;
if (props.vpc && selectedSubnets && selectedSubnets.subnetIds.length === 0) {
throw new cdk.ValidationError(
'No subnets matched the given subnetSelection for the provided VPC.', this,
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the first validation on checking length of selected subnets
e4d2666 to
b523ba3
Compare
Pull request has been modified.
b523ba3 to
105746f
Compare
… Infrastructure Configuration
105746f to
e8c029f
Compare
… Infrastructure Configuration
| } | ||
|
|
||
| const placement: CfnInfrastructureConfiguration.PlacementProperty = { | ||
| ...(props.ec2InstanceAvailabilityZone && { availabilityZone: props.ec2InstanceAvailabilityZone }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this to !==undefined to handle CDK tokens safely:
const placement =
props.ec2InstanceAvailabilityZone !== undefined ||
props.ec2InstanceHostId !== undefined ||
props.ec2InstanceHostResourceGroupArn !== undefined ||
props.ec2InstanceTenancy !== undefined
? {
...(props.ec2InstanceAvailabilityZone !== undefined && { availabilityZone: props.ec2InstanceAvailabilityZone }),
...(props.ec2InstanceHostId !== undefined && { hostId: props.ec2InstanceHostId }),
...(props.ec2InstanceHostResourceGroupArn !== undefined && { hostResourceGroupArn: props.ec2InstanceHostResourceGroupArn }),
...(props.ec2InstanceTenancy !== undefined && { tenancy: props.ec2InstanceTenancy }),
}
: undefined;
| * | ||
| * @default - 2 | ||
| */ | ||
| readonly httpPutResponseHopLimit?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need validation on the max value for HttpPutResponseHopLimit ? CFN states max as 64.
| public readonly instanceProfile: iam.IInstanceProfile; | ||
|
|
||
| /** | ||
| * The role associateded with the EC2 instance profile used for the build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type: associateded → associated
| this.logBucket = props.logging?.s3Bucket; | ||
|
|
||
| if (this.logBucket && this.role && props.logging?.s3KeyPrefix !== undefined) { | ||
| this.logBucket.grantPut(this.role, `${props.logging.s3KeyPrefix}/*`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since s3KeyPrefix is optional, if user sets logging.s3Bucket without s3KeyPrefix- Image Builder may then fail to upload logs due to msising write permissions; we can change this grant back to:
this.logBucket.grantPut( this.role, props.logging?.s3KeyPrefix ? `${props.logging.s3KeyPrefix}/*` : '*',`
| }).toThrow(cdk.ValidationError); | ||
| }); | ||
|
|
||
| test('throws a validation error an a subnet selection is provided without a VPC', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: throws validation error when subnet selection is provided without a VPC
Issue # (aws/aws-cdk-rfcs#789)
Reason for this change
This change adds a new alpha module for EC2 Image Builder L2 Constructs (
@aws-cdk/aws-imagebuilder-alpha), as outlined in aws/aws-cdk-rfcs#789. This PR specifically implements theInfrastructureConfigurationconstruct.Description of changes
This change implements the
InfrastructureConfigurationconstruct, which is a higher-level construct ofCfnInfrastructureConfiguration.Note - I have also added the YAML library as a dependency to the module. This will be used for the component/workflow resources, which need to pass JSON objects in a YAML string format when creating the resource.
Example
Describe any new or updated permissions being added
N/A - new L2 construct in alpha module
Description of how you validated changes
Validated with unit tests and integration tests. Manually verified generated CFN templates as well.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license